Skip to content

fix(review): scan added lines whose content starts with ++ for secrets - #5952

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
andriypolanski:fix-secrets-scan-plusplus-added-lines-5942
Jul 15, 2026
Merged

fix(review): scan added lines whose content starts with ++ for secrets#5952
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
andriypolanski:fix-secrets-scan-plusplus-added-lines-5942

Conversation

@andriypolanski

@andriypolanski andriypolanski commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Closes #5942

PR body

Summary

scanDiffForSecretsWithLocations had a leftover !line.startsWith("+++") guard
meant to skip unified-diff +++ b/path file headers. This scanner never sees those
headers — file boundaries are already owned by DIFF_FILE_HEADER_PATTERN
(### path (status) +N/-N). The guard's only live effect was to misclassify a
genuine added line whose content starts with ++ (after the diff + marker
the line looks like +++…), fall through to the context-line path, and never
call matchedKindsIn. A committed secret on such a line silently bypassed the
unconditional secret_leak hard blocker.

Change

  • src/review/secrets-scan.ts: remove the stale !line.startsWith("+++")
    guard so any line starting with + is scanned as an added line. All other
    classifications (DIFF_FILE_HEADER_PATTERN, DIFF_HUNK_HEADER_PATTERN,
    --prefixed removals, context lines) are unchanged. matchedKindsIn and the
    file-header path-scanning branch are untouched.

Tests (test/unit/secrets-scan.test.ts)

  • Regression: a diff with an added line +++ token: ghp_… +++ is detected as
    github_token at the correct file:line.
  • Ordinary added/removed/context classification still works (secret on a normal
    + line; removed/ lines unscanned).
  • ### path (status) +N/-N file headers are not falsely scanned as content
    lines (line > 0).
  • Added/renamed path attribution via the header (line: 0) still works.

Coverage

src/review/secrets-scan.ts remains at full patch coverage for the touched
lines (the change deletes a branch condition; the new tests exercise the remaining
startsWith("+") path including content that previously tripped the guard).

Links

@andriypolanski
andriypolanski marked this pull request as draft July 14, 2026 23:55
@superagent-security

superagent-security Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@loopover-orb loopover-orb Bot added gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. gittensor:priority Maintainer-selected Gittensor priority — scores a 1.5x multiplier. labels Jul 15, 2026
@loopover-orb

loopover-orb Bot commented Jul 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-15 04:56:49 UTC

2 files · 1 AI reviewer · no blockers · readiness 80/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This removes a stale `!line.startsWith("+++")` guard in scanDiffForSecretsWithLocations that was silently skipping added lines whose content happens to begin with `++` (e.g. `+++ token: ghp_... +++`), letting a real secret bypass the secret_leak hard blocker. The fix is well-scoped: it's a one-line condition removal, the description correctly traces that this scanner's diff format uses `### path (status) +N/-N` headers (not real unified-diff `+++`/`---` markers) so the guard had no legitimate purpose here, and the new tests exercise the exact previously-missed case plus regression coverage for ordinary added/removed/context lines and file-header non-scanning.

Nits — 4 non-blocking
  • src/review/secrets-scan.ts: the added inline comment is quite long (6 lines) for a one-line condition change — could be trimmed to the key fact (this scanner never emits real unified-diff `+++`/`---` headers) with the issue reference.
  • test/unit/secrets-scan.test.ts: worth adding a case for a line starting with `---` content (e.g. `+---secret---`) to confirm no analogous latent guard exists for removed-line markers, even though the diff doesn't show one.
  • Consider a short code comment at the top of the function (not just at this branch) documenting that this scanner's diff format uses `### path (status)` headers instead of standard unified-diff markers, so future readers don't reintroduce a `+++`/`---` guard by analogy to real diff parsing.
  • The PR description's coverage claim (full patch coverage) is plausible given the tests shown, but confirm CI coverage output rather than taking the claim at face value.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #5942
Related work ⚠️ 2 scoped overlaps Top overlaps are listed below; lower-confidence bulk is hidden.
Change scope ❌ 8/20 High review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 198 registered-repo PR(s), 117 merged, 25 issue(s).
Contributor context ✅ Confirmed Gittensor contributor andriypolanski; Gittensor profile; 198 PR(s), 25 issue(s).
Gate result ✅ Passing No configured blocker found.
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Review context
Contributor next steps
  • Start here: Review top overlaps.
  • Then work through the remaining 2 steps in the Signals table above.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://gittensory.aethereal.dev/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed

💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@codecov

codecov Bot commented Jul 15, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@37b0882). Learn more about missing BASE report.
⚠️ Report is 29 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #5952   +/-   ##
=======================================
  Coverage        ?   95.14%           
=======================================
  Files           ?      589           
  Lines           ?    46605           
  Branches        ?    14889           
=======================================
  Hits            ?    44344           
  Misses          ?     1511           
  Partials        ?      750
Flag Coverage Δ
shard-1 43.26% <100.00%> (?)
shard-2 36.36% <100.00%> (?)
shard-3 31.90% <100.00%> (?)
shard-4 31.87% <100.00%> (?)
shard-5 32.45% <0.00%> (?)
shard-6 44.89% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/review/secrets-scan.ts 100.00% <100.00%> (ø)

@andriypolanski
andriypolanski marked this pull request as ready for review July 15, 2026 04:46
loopover-orb[bot]
loopover-orb Bot approved these changes Jul 15, 2026
View reviewed changes

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoopOver approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit fe331a9 into JSONbored:main Jul 15, 2026
19 of 26 checks passed
@nghetienhiep nghetienhiep mentioned this pull request Jul 15, 2026
Closed
This was referenced Jul 15, 2026
Closed
Closed
This was referenced Jul 15, 2026
Closed
Closed
@andriypolanski
andriypolanski deleted the fix-secrets-scan-plusplus-added-lines-5942 branch July 16, 2026 15:10
@github-actions github-actions Bot mentioned this pull request Jul 17, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@loopover-orb loopover-orb[bot] loopover-orb[bot] approved these changes
@JSONbored JSONbored Awaiting requested review from JSONbored JSONbored is a code owner

Assignees

@andriypolanski andriypolanski

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. gittensor:priority Maintainer-selected Gittensor priority — scores a 1.5x multiplier.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

fix(review): stale +++ file-header guard in scanDiffForSecretsWithLocations bypasses secret scanning on added lines starting with ++

3 participants

@andriypolanski @JSONbored @andriy-polanski